perf: Optimize NULL handling in StringViewArrayBuilder#21538
perf: Optimize NULL handling in StringViewArrayBuilder#21538martin-g merged 5 commits intoapache:mainfrom
StringViewArrayBuilder#21538Conversation
|
FYI @Omega359 |
Omega359
left a comment
There was a problem hiding this comment.
I looked this over and it seems reasonable, thanks!
|
@martin-g @timsaucer Thanks for the reviews! I noticed that the existing code panicked for large input strings, so I adjusted that to return an error (via |
|
run benchmark concat |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-string-view-builder-nulls (4e33f61) to 4389f14 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
@martin-g Is this okay to merge? |
|
Thanks @martin-g @timsaucer ! |
| } | ||
|
|
||
| pub fn append_offset(&mut self) { | ||
| pub fn append_offset(&mut self) -> Result<()> { |
There was a problem hiding this comment.
I think this is an API change we should call out (I'll add a api-change label)
Which issue does this PR close?
StringViewArrayBuilder#21537.Rationale for this change
StringViewArrayBuilderis implemented on top of Arrow'sStringViewBuilder; the latter tracks NULLs incrementally. However, theStringViewArrayBuilderrequires callers to pass a NULL buffer tofinish()anyway, so the NULL bitmap that has been computed byStringViewBuilderis discarded. It would be more efficient to stop usingStringViewBuilderso that we don't do this redundant work; in theory there might be room for inconsistency between the two NULL bitmaps as well.Right now,
StringViewArrayBuilderis only used by theconcatandconcat_wsUDFs, but I'd like to generalize the API and use it more broadly in place ofStringViewBuilder(#21539). For the time being, here are the results of this PR on theconcatbenchmarks (Arm64):What changes are included in this PR?
StringViewBuilderand build the views ourselvesAre these changes tested?
Yes.
Are there any user-facing changes?
No.